Skip to content

feat(workspace): offer to install the engine a bound workspace needs - #1169

Draft
ralphstodomingo wants to merge 10 commits into
feat/workspace-precedence-v2from
feat/workspace-install-offer-v2
Draft

feat(workspace): offer to install the engine a bound workspace needs#1169
ralphstodomingo wants to merge 10 commits into
feat/workspace-precedence-v2from
feat/workspace-install-offer-v2

Conversation

@ralphstodomingo

@ralphstodomingo ralphstodomingo commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

Issue for this PR

Closes #1157

Type of change

  • Bug fix
  • New feature
  • Refactor / code improvement
  • Documentation

What does this PR do?

Stacked on #1168 (which stacks on #1167) — this PR is the commit on top. It is the install offer from #1158 restacked onto the overlay attach; the dialog and its rules are unchanged, the refusal it substitutes is now the overlay's.

A bound workspace whose declared tools need the local engine used to report a missing engine as a 10-second toast with a command in it. This replaces that with an offer: Install now / Copy command / Not now. The install only ever runs from an explicit choice — the attach never installs anything on its own.

How the offer reaches the TUI: the plugin runtime loads plugins in a separate module realm, so nothing in-process can hand it the offer. It is published on the event bus, which is what toasts already use, and since that event carries no payload the plugin re-derives the detail from the binding on disk and the engine on PATH (describeOffer, state-free by design). After a successful install the plugin does not reconcile anything itself — it cannot reach the server's module — and does not need to: the turn boundary looks for a missing engine on PATH again every turn, so the engine attaches on the next message.

Deliberate details, carried over from the original:

  • One select for every phase, with sentinel rows for installing/installed/failed; swapping the top-level component would drop focus.
  • Install now only appears when Node 20+ and npm are on PATH; otherwise the command is shown with the reason it cannot be run for you.
  • Copy command reads the clipboard back before claiming success; the host's writer swallows backend failures.
  • Not now latches 7 days, keyed on the workspace id, scoped by tenant and API URL.
  • Headless run prints one stderr line instead; stdout stays reserved for JSON events. An attached run renders the notice in its own event loop.
  • Both too-old exits route through the offer. The install has a real deadline via an abort signal, reports its result even if the dialog was dismissed while npm ran, and only one offer is raised at a time.
  • A zero exit from npm is not trusted: the installed engine must be discoverable on PATH and clear the version floor.
  • The headless marker is stripped from the environment of nested entrypoints started by the bash tool, so a child with a TUI still gets the dialog.

How did you verify your code works?

bun run typecheck clean. New offer suite (routing to surface / bus / toast / headless line, too-old with the found version, healthy engine never offered, describeOffer re-derivation, stderr-not-stdout, install deadline via abort signal, install success verified not assumed) plus the plugin suite; 357 tests pass across the workspace, plugin, precedence and MCP lifecycle suites on this stack.

End-to-end rows from #1158 (dialog with all three options and a live declared count; Install now into an isolated prefix with tools on the next message in the same session; Not now suppresses and returns after the latch ages; no Node hides Install now; headless prints exactly one line) are re-run on this stack and recorded in the review-log comment below before this leaves draft.

Not verified here: the successful clipboard path (this host has no clipboard backend); Windows (npm.cmd follows existing precedent in this repo).

Screenshots / recordings

Terminal UI; captured pane output will accompany the E2E rows on the review-log comment.

Checklist

  • I have tested my changes locally
  • I have not included unrelated changes in this PR

Summary by cubic

Replaces the toast that printed an install command for a workspace with a missing or too-old engine with an interactive offer: Install now / Copy command / Not now. The install runs only from an explicit choice, and the installed engine attaches on the next message without a restart.

Offer behavior

  • Install now appears only when Node 20+ and npm are on PATH; otherwise the command shows with the reason it cannot run.
  • Copy command reads the clipboard back before claiming success.
  • Not now latches 7 days per workspace, scoped by tenant and API URL; past the latch the offer is re-raised hourly.
  • The dialog waits for the TUI's persisted settings to load before checking the latch, so a restart can't re-offer inside a still-active silence.
  • Headless runs print one stderr line per verdict per process; stdout stays reserved for JSON events, and an attached run prints only the offer raised for its own session.
  • npm's zero exit is not trusted — the installed engine must be on PATH and clear the version floor.
  • The install runs with a hard deadline over npm's whole process tree (SIGTERM, then SIGKILL, which stays armed after npm exits).
  • A too-old engine is re-probed as soon as the file on PATH changes, so an in-place update attaches on the next message.
  • The offer dialog hides the filter box, so keystrokes during the install reach the prompt instead of collecting in the dialog.
  • The bash tool strips the headless marker from nested entrypoints so children with a TUI still render the dialog.

Written for commit 089bb62. Summary will update on new commits.

Review in cubic

@coderabbitai

coderabbitai Bot commented Aug 27, 2026

Copy link
Copy Markdown

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

  • Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@ralphstodomingo

ralphstodomingo commented Aug 27, 2026

Copy link
Copy Markdown
Contributor Author

Review log

Stacked on #1168#1167#1167's review-log comment carries the attach contract; this PR is reviewed the same way, with capped rounds.

Claims (in addition to the stack's)

  1. The attach never installs anything on its own; installEngine runs only from an explicit "Install now".
  2. Exactly one surface says a refusal once per session per verdict: the dialog when a TUI plugin is present, one stderr line headless, the toast only when the bus is unavailable.
  3. Install now appears only with Node 20+ and npm on PATH; a zero exit from npm is not trusted — the engine must be discoverable on PATH and clear the floor.
  4. One offer at a time; an install that outlives its dialog still reports its result; Not now latches 7 days per workspace.
  5. After an install, the next turn boundary picks the engine up without a restart (the plugin cannot reach the server's module; the boundary looks for a missing engine on PATH every turn).

Disclosed residuals

  • The successful clipboard path is unverified on this host (no clipboard backend).
  • Windows npm.cmd follows existing precedent in this repo rather than a test.
  • Under run --attach, an offer published after the run goes idle cannot render.
  • attach <url> runs the plugin on the client; a client with the same path and its own binding is misread.

End-to-end rows (from #1158, re-run on this stack before this leaves draft)

# Scenario Result
1 engine off PATH, bound dir → dialog with three options and a live declared count ✓ live TUI on the stack (2026-08-28): dialog on the first message with Install now / Copy command / Not now, "declares 52 integration tools", the exact command
2 Install now (isolated prefix) → tools on the next message, same session ✓ Install now ran npm i -g into an isolated prefix (npm_config_prefix), engine 0.7.0 on that prefix's bin after 16 s, success toast; the next message in the same session spawned the engine from that prefix pinned to the workspace and listed its tools; no project config written
3 Copy command → clipboard unverifiable on this host
4 Not now → suppressed; latch aged 8 days → dialog returns ✓ Not now closed the dialog; the next message in the same session and a later session in the same sandbox raised no dialog (latch recorded in the plugin kv, keyed on the workspace); the aged-latch return is unit-tested
5 no Node on PATH → command shown, no Install now ✓ dialog with the command and "(needs Node 20+ to install — Node was not found on PATH)"; only Copy command / Not now offered
6 headless run → exactly one stderr line ✓ (row 6 of #1167, same code path)

Rounds

(none yet)

Codex rounds

round head findings outcome
1 58a3f4ca6 1 (P2): a session alive past the 7-day "Not now" latch was never re-offered — announceRefusal's per-session dedupe kept the offer route silent, so the TUI's latch check was unreachable (violated claim 4) fixed in 9bc01a5d2: the offer route's dedupe expires on OFFER_SKIP_TTL_MS, defined once in engine-offer and shared with the plugin latch; regression test "a session that outlives the Not-now latch is offered again"
2 9bc01a5d2 1 (P2): a too-old engine updated in place by "Install now" stayed refused on the next message — the too-old probe memo was honoured for 30 s by path alone, and invalidateProbe() had no caller (the dialog runs in another module realm; violated claim 5) fixed in 63ba0a1f9: the memo carries the binary's fingerprint (size + mtime, symlinks followed) and ends when it changes; un-stat-able falls back to the TTL; regression test "a too-old engine updated in place is re-probed on the next turn, inside the TTL"
3 63ba0a1f9 2 (P2): (a) the attach-side re-offer window ran from when the offer was raised while the TUI latch runs from Not now, so a dialog left open could hold a long-lived session for a second full window (claim 4); (b) the attached-run stderr line accepted the offer command with no session id, so another session's offer in the same directory printed too (claim 2) fixed in afa3fda4a: (a) past the window the offer is re-raised every OFFER_RECHECK_MS (1 h) and the TUI suppresses until its latch ends — bounded to ≤ 1 h past the latch; (b) CommandExecute carries an optional sessionID (marker block in tui-event.ts), publishOffer sets it, run --attach prints only its own session's offer. Tests updated ("…naming the session it is for", hourly re-raise).
afa3fda4a65b2916f2 rebase only, onto #1168's 893ff8f93 (marker-form fix there); the four commits are unchanged strict marker guard passes for the whole stack against main; 309 workspace/plugin tests pass on the rebased head
4 65b2916f2 1 (P2): a headless run whose agent spawns a sub-agent printed the refusal line twice — the child session's first catalog settled the same verdict and announceRefusal dedupes per session (claim 2) fixed in a5f7cd168: the headless line is deduplicated per verdict per process; test "headless, a sub-agent's session in the same process prints nothing more". Rounds continue at the author's request until clean.
5 a5f7cd168 1 (P2): the TUI read the "Not now" latch from api.kv before the store had hydrated from kv.json, so the first message after a restart could show the dialog inside the seven days (claim 4) fixed in 464ef949d: the offer awaits api.kv.ready (polled every 25 ms, bounded at 3 s, then proceeds as before with a warning); tests "engine install offer — kv hydration" (waits, immediate when ready, gives up on timeout). The older post-scan latch on main has the same hazard and is left for a follow-up.
6 464ef949d 1 (P2): at the five-minute deadline only npm's own pid was signalled and the run settled on close, so a descendant holding the stderr pipe kept "Install now" on "Installing…" indefinitely (claim 4's install deadline) fixed in 26831a2ab: runInstall spawns npm in its own process group, settles on the child's exit, and at the deadline signals the group (SIGTERM, SIGKILL after 5 s) before reporting the timeout; Process.run no longer used for the install. Real-process tests: a straggler holding stderr does not hold the run; a TERM-ignoring tree is terminated and reported as timed out.
7 26831a2ab 1 (P2): the deadline's SIGTERM kills npm (the group leader) and its exit cleared the SIGKILL timer, so a TERM-ignoring descendant survived the reported timeout (claim 4's deadline) fixed in 2f6fd27db: once the deadline has fired the escalation stays armed and SIGKILLs the process group after the grace even though the leader is gone. Real-process test: a straggler that ignores SIGTERM is gone after the grace. Context: the published engine's only install-time script is a guarded postinstall that is a no-op outside a git checkout, so these trees are adversarial rather than observed — the deadline is now correct for them anyway.

@ralphstodomingo

Copy link
Copy Markdown
Contributor Author

@codex review against the numbered claims and the disclosed residuals in the review-log comment on this PR: report only a reproducible trace that violates a numbered claim; an instance of a listed residual is disclosed behaviour, not a finding.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 58a3f4ca63

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread packages/opencode/src/altimate/workspace/engine-overlay.ts Outdated
@ralphstodomingo

Copy link
Copy Markdown
Contributor Author

@codex review against the numbered claims and the disclosed residuals in the review-log comment on this PR: report only a reproducible trace that violates a numbered claim; an instance of a listed residual is disclosed behaviour, not a finding.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 9bc01a5d2a

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread packages/opencode/src/altimate/workspace/engine-overlay.ts Outdated
@ralphstodomingo

Copy link
Copy Markdown
Contributor Author

@codex review against the numbered claims and the disclosed residuals in the review-log comment on this PR: report only a reproducible trace that violates a numbered claim; an instance of a listed residual is disclosed behaviour, not a finding.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 63ba0a1f93

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread packages/opencode/src/altimate/workspace/engine-overlay.ts Outdated
Comment thread packages/opencode/src/cli/cmd/run.ts
@ralphstodomingo
ralphstodomingo force-pushed the feat/workspace-install-offer-v2 branch from afa3fda to 65b2916 Compare August 27, 2026 22:11
@ralphstodomingo

Copy link
Copy Markdown
Contributor Author

@codex review against the numbered claims and the disclosed residuals in the review-log comment on this PR: report only a reproducible trace that violates a numbered claim; an instance of a listed residual is disclosed behaviour, not a finding.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 65b2916f2a

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread packages/opencode/src/altimate/workspace/engine-offer.ts
@ralphstodomingo

Copy link
Copy Markdown
Contributor Author

@codex review against the numbered claims and the disclosed residuals in the review-log comment on this PR: report only a reproducible trace that violates a numbered claim; an instance of a listed residual is disclosed behaviour, not a finding.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a5f7cd1685

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

// engine appeared, or the project is no longer bound. Say nothing.
if (!offer) return release()
const latchScope = await currentLatchScope()
if (isEngineSkipActive(api, offer.workspaceId, latchScope, Date.now())) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Wait for KV hydration before checking the skip latch

Repro: choose Not now, restart the TUI, delay the persisted KV read (for example, hold the tui-kv Flock), and immediately send a message that raises the engine offer. KVProvider initially exposes an empty store with api.kv.ready === false, but this check treats the missing record as an expired latch and opens the dialog; later hydration does not re-check or close it. Thus a persisted Not-now choice can be ignored well within seven days, violating the numbered suppression claim.

Useful? React with 👍 / 👎.

@ralphstodomingo

Copy link
Copy Markdown
Contributor Author

@codex review against the numbered claims and the disclosed residuals in the review-log comment on this PR: report only a reproducible trace that violates a numbered claim; an instance of a listed residual is disclosed behaviour, not a finding.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 464ef949da

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread packages/opencode/src/altimate/workspace/engine-offer.ts Outdated
@ralphstodomingo

Copy link
Copy Markdown
Contributor Author

@codex review against the numbered claims and the disclosed residuals in the review-log comment on this PR: report only a reproducible trace that violates a numbered claim; an instance of a listed residual is disclosed behaviour, not a finding.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 26831a2ab8

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread packages/opencode/src/altimate/workspace/engine-offer.ts Outdated
@ralphstodomingo

Copy link
Copy Markdown
Contributor Author

@codex review against the numbered claims and the disclosed residuals in the review-log comment on this PR: report only a reproducible trace that violates a numbered claim; an instance of a listed residual is disclosed behaviour, not a finding.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2f6fd27db3

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +1510 to +1512
if (!(await awaitKvReady(api.kv))) {
log.warn("kv store not hydrated in time; checking the engine install latch against what is loaded")
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Suppress the offer when KV hydration times out

Repro: choose Not now, restart the TUI, keep the tui-kv Flock held for more than three seconds, and immediately send a message. Fresh evidence after the prior fix is that awaitKvReady() now returns false after a fixed timeout, but this branch merely logs and then consults the still-empty store, so the dialog opens within the seven-day suppression period; later hydration does not revisit or close it. On timeout, release/drop this offer and allow a later raise after KV becomes ready instead of treating unhydrated state as an absent latch.

Useful? React with 👍 / 👎.

@ralphstodomingo
ralphstodomingo force-pushed the feat/workspace-precedence-v2 branch from 893ff8f to b8daef2 Compare August 28, 2026 00:48
ralphstodomingo added 5 commits August 28, 2026 08:48
A bound workspace whose declared tools need the local engine used to report a
missing engine as a toast with a command in it. This replaces that with an
offer: Install now / Copy command / Not now. The install only ever runs from
an explicit choice; the turn boundary picks the installed engine up on the
next message. The offer crosses to the TUI over the event bus and is
re-derived there; headless prints one stderr line.
…now latch

`announceRefusal` deduped the offer once per session per verdict, so a session
open for longer than the 7-day "Not now" latch never saw the offer again — the
TUI's latch check was unreachable. The offer route's dedupe now expires on the
same TTL, `OFFER_SKIP_TTL_MS`, defined once in `engine-offer` and shared with
the plugin's latch. Toast-only refusals keep "once per session".
The too-old probe memo was honoured for 30s by path alone, so an engine
updated in place by the offer's `npm i -g` stayed refused on the next message
— and nothing could invalidate the memo, since the offer runs in another module
realm. The memo now also carries the binary's fingerprint (size + mtime, symlinks
followed) and ends when it changes; an un-stat-able binary falls back to the TTL.
…hed-run line to its session

The attach-side dedupe expired seven days after the offer was raised, but the
TUI latch runs from "Not now", which can come later — a long-lived session
could wait a second full window. Past the window the offer is now re-raised
every `OFFER_RECHECK_MS` and the TUI suppresses it until its latch ends.

The offer command now carries the session it was raised for, so an attached
headless run prints only its own offer, not another session's in the directory.
…ession

A headless `run` whose agent uses the task tool creates a child session, and
that session's first catalog settled the same verdict and printed the same
stderr line again. The headless line is now deduplicated per verdict across
the process; sessions still track their own announcement for the toast and
offer surfaces.
ralphstodomingo added 4 commits August 28, 2026 08:49
…ow latch

The store is empty until kv.json has been read, so an offer raised on the
first message after a restart read a persisted "Not now" as absent and showed
the dialog inside the seven days. `api.kv.ready` is a plain getter, so the
offer polls it (25 ms, bounded at 3 s) and on timeout proceeds as before.
… tree

`Process.run` settles on `close`, and the abort only signalled npm's own pid, so
a descendant that kept the stderr pipe open held `installEngine` past the five
minutes and the dialog stayed on "Installing…". The install now runs in its
own process group, settles on the child's `exit`, and at the deadline signals
the group — SIGTERM, then SIGKILL after a grace — before reporting the timeout.
npm, the group leader, normally dies on the deadline's SIGTERM, and its exit
cleared the escalation timer — so a descendant that ignored SIGTERM outlived
the reported timeout. Once the deadline has fired the SIGKILL to the process
group stays scheduled; the group outlives its leader while any member is alive.
@ralphstodomingo
ralphstodomingo force-pushed the feat/workspace-install-offer-v2 branch from 2f6fd27 to b645742 Compare August 28, 2026 00:52
The dialog already skipped filtering (`skipFilter`); the box itself only
collected stray keystrokes while the install ran. `renderFilter` is passed
through the plugin dialog API to the component that already supports it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant